Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes incorrect handling of ADT's drop requirements #90218

Merged
merged 3 commits into from
Oct 28, 2021

Conversation

JakobDegen
Copy link
Contributor

@JakobDegen JakobDegen commented Oct 23, 2021

Fixes #90024 and a bunch of duplicates.

The main issue was just that the contract of NeedsDropTypes::adt_components was inconsistent; the list of types it might return were the generic parameters themselves or the fields of the ADT, depending on the nature of the drop impl. This meant that the caller could not determine whether a .subst() call was still needed on those types; it called .subst() in all cases, and this led to ICEs when the returned types were the generic params.

First contribution of more than a few lines, so feedback definitely appreciated.

@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 23, 2021
Uses 2 MCVEs from the issue tracker that test opposite sides of the problem.
@JakobDegen JakobDegen marked this pull request as ready for review October 24, 2021 02:39
@wesleywiser
Copy link
Member

r? @nikomatsakis

Copy link
Member

@arora-aman arora-aman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix.

LGTM, nits only.

compiler/rustc_ty_utils/src/needs_drop.rs Outdated Show resolved Hide resolved
compiler/rustc_ty_utils/src/needs_drop.rs Outdated Show resolved Hide resolved
compiler/rustc_ty_utils/src/needs_drop.rs Outdated Show resolved Hide resolved
@JakobDegen
Copy link
Contributor Author

Cleaned those up and fixed the function names on a couple others, doing this before had slipped my mind. Thanks!

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 28, 2021
@pnkfelix
Copy link
Member

nominating because @apiraino was concerned this might get overlooked

@apiraino
Copy link
Contributor

apiraino commented Oct 28, 2021

Adding note of the Zulip thread

@rustbot label -I-nominated +I-compiler-nominated

@rustbot rustbot added I-compiler-nominated Nominated for discussion during a compiler team meeting. and removed I-nominated labels Oct 28, 2021
@nikomatsakis
Copy link
Contributor

@bors r+

Thanks for the comment explaining the purpose of the PR! Always helpful.

@bors
Copy link
Contributor

bors commented Oct 28, 2021

📌 Commit aff37f8 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 28, 2021
@ehuss
Copy link
Contributor

ehuss commented Oct 28, 2021

@bors p=1

To help stem the tide of reports.

@bors
Copy link
Contributor

bors commented Oct 28, 2021

⌛ Testing commit aff37f8 with merge 85c0558...

@ehuss ehuss added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 28, 2021
@bors
Copy link
Contributor

bors commented Oct 28, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 85c0558 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 28, 2021
@bors bors merged commit 85c0558 into rust-lang:master Oct 28, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 28, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (85c0558): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

  • Very large regression in instruction counts (up to 5.3% on full builds of regression-31157)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@apiraino
Copy link
Contributor

The perf report suggests a significant regression on incremental compilation scenarios (if I correctly read the report): something worth investigating before backporting?

self.param_env,
required_ty.subst(tcx, substs),
);
let subst_ty =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I probably would have renamed this variable. I think the reason it was named subst_ty is because it, prior to this PR, had the substs applied to it. But that is no longer the case here, if I understand correctly.

@pnkfelix
Copy link
Member

pnkfelix commented Nov 2, 2021

Visiting for performance triage this week.

  • regression-31157 check regressed by 4.7% on incr and 5.3 on full.
  • issue-46449 check regressed by 1.89% on incr-full.
  • wg-grammar regressed by 1.3-1.4% in a bunch of scenarios.
  • but otherwise, this does not seem too bad. I think we should keep this PR approved for backport, while also look into fixing the regression on nightly.

@pnkfelix
Copy link
Member

pnkfelix commented Nov 2, 2021

Filed issue #90504 for follow-up investigation.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Nov 2, 2021
@apiraino
Copy link
Contributor

apiraino commented Nov 2, 2021

thanks @pnkfelix !

@pnkfelix
Copy link
Member

pnkfelix commented Nov 4, 2021

discussed in T-compiler meeting. backport approved.

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 4, 2021
@JakobDegen JakobDegen deleted the adt_significant_drop_fix branch November 11, 2021 18:25
@cuviper cuviper mentioned this pull request Nov 16, 2021
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 16, 2021
@cuviper cuviper modified the milestones: 1.58.0, 1.57.0 Nov 16, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 16, 2021
…lacrum

Address performance regression introduced by rust-lang#90218

As part of the changes in rust-lang#90218 , the `adt_drop_tys` and friends code stopped recursing through the query system, meaning that intermediate computations did not get cached. This change adds the recursions back in without re-introducing any of the old issues.

On local benchmarks this fixes the 5% regressions in rust-lang#90504 ; the wg-grammar regressions didn't seem to move too much. I may take some time later to look into those.

Not sure who to request for review here, so will leave it up to whoever gets it.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 19, 2021
[beta] backports

-  Fix assertion failures in OwnedHandle with windows_subsystem. rust-lang#88798
-  Ensure that pushing empty path works as before on verbatim paths rust-lang#89665
-  Feature gate + make must_not_suspend allow-by-default rust-lang#89826
-  Only use clone3 when needed for pidfd rust-lang#89930
-  Fix documentation header sizes rust-lang#90186
-  Fixes incorrect handling of ADT's drop requirements rust-lang#90218
-  Fix ICE when forgetting to Box a parameter to a Self::func call rust-lang#90221
-  Prevent duplicate caller bounds candidates by exposing default substs in Unevaluated rust-lang#90266
-  Update odht crate to 0.3.1 (big-endian bugfix) rust-lang#90403
-  rustdoc: Go back to loading all external crates unconditionally rust-lang#90489
-  Split doc_cfg and doc_auto_cfg features rust-lang#90502
-  Apply adjustments for field expression even if inaccessible rust-lang#90508
-  Warn for variables that are no longer captured rust-lang#90597
-  Properly register text_direction_codepoint_in_comment lint. rust-lang#90626
-  CI: Use ubuntu image to download openssl, curl sources, cacert.pem for x86 dist builds rust-lang#90457
-  Android is not GNU rust-lang#90834
-  Update llvm submodule rust-lang#90954

Additionally, this bumps the stage 0 compiler from beta to stable 1.56.1.

r? `@Mark-Simulacrum`
@apiraino apiraino removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2021 cargo fix --edition ICE: type parameter out of range when substituting